fix: dashboard various fixes #78
Conversation
* Context: React native web sends base64 images that cannot be parsed by mutler
* Rename ratings to reviews * Use ratings only for professionalism/communication/reliability * Create reviews controller and service * Remove route with */self.
* change id from int to string * add state * add favorite review model
…r controller here
adelinaenache
force-pushed
the
dashboard-various-fixes
branch
from
4d28b2e to
859906c
Compare
* Add authType property on connectionDto * remove jobTitle property. Use headline everywhere
| /** | ||
| * Create a connection between current User and another user. | ||
| */ | ||
| @Post('/connect/:userId') |
There was a problem hiding this comment.
I think a better name for this would be add. Similarly, for the delete endpoint it can be remove. This will look pretty meaningful when put together:
GET /api/connection/add/:userIdDELETE /api/connection/remove/:userId
There was a problem hiding this comment.
The way it is right now it is in alignment with RESTful standards. The verb "POST" signifies a resource creation, and DELETE signifies a resource removal.
Adding verbs like add and remove in the endpoint path can be redundant and not as clean as using HTTP methods to indicate the action. RESTful best practices suggest using nouns in URLs and verbs in HTTP methods.
https://mglaman.dev/blog/post-or-put-patch-and-delete-urls-are-cheap-api-design-matters
https://www.mscharhag.com/api-design/rest-deleting-resources
There was a problem hiding this comment.
True, I know it is bit of a gamble in here. The thing is, feels that the connect word might specify an action and not a resource. Truly we can get rid of add and delete since the methods will signify that. I think we can go with /api/connection/:userId as the path and DELETE and POST as the methods?
| import { Public } from 'src/decorators/public.decorator'; | ||
|
|
||
| @ApiBearerAuth() | ||
| @Controller('connections') |
There was a problem hiding this comment.
Our existing endpoints use singular in the controller name, so we should be sticking to connection in here.
There was a problem hiding this comment.
This is a larger discussions. I think it makes sense to have user since in that controller we're usually dealing with a single user.
Put in reviews and connections we are usually dealing with multiple resources. See: Google's api design guidelines
and
this StackOverflow discussion
| /** | ||
| * Get a connection. | ||
| */ | ||
| @Get('/:userId') |
There was a problem hiding this comment.
I'm not entirely sure about this usecase. Can you link a screenshot/url?
There was a problem hiding this comment.
It gets a single "connection" https://www.figma.com/design/p4gfqRDn3ma4zPaPAj8vH4/Culero?node-id=46-2936&t=pqBwKPkw3kB9HieJ-4
| @Public() | ||
| @ApiBadRequestResponse() | ||
| async searchUser( | ||
| @Param('query') query: string, |
There was a problem hiding this comment.
Maybe you can add an alphanumeric regex check in here so that users cant use SQL injection?
There was a problem hiding this comment.
If prisma wouldn't be Sql injection safe out of the box all of our endpoint would be vulnerable for such a thing since we're not doing any kind of prevention for this anywhere.
Glad that is safe out of the box >.<
https://stackoverflow.com/questions/73943274/prisma-findunique-is-sql-injection-safe
src/prisma/migrations/20240521101330_favorite_by_to_favorites/migration.sql
Outdated
Show resolved
Hide resolved
It does make sense to use. DTO because:
|
…and doesn't see lowering the case of the file as an actual change)
Okay! Cool! Would you think it would be good idea to seperate the request and response dtos in two separate folders under the dto folder? |
No, not necessarily. I feel like that would just create more overhead in the development since we usually have just one output DTO (that is why I implemented that |
The point im trying to make here is, say the number of endpoints grow. For the simplest of example, lets take the example of retrieving a list of items, and a specific item. For a specific item, you would want to have much more detailed data, and sometimes even nested data. For lists, you wont bother sending the entire thing, but just partial information that is needed by the UI. So if i call this entity as
When we are trying to create resources, we will be sending in details like name, description, etc etc. We wont be having any ID or any nested mappings (most of the time). In this case, if we are relying on the same DTO for both creating and fetching our records, we will land up in trouble where the swagger parser will misunderstand what exactly it is that we need. Also, it won't make sense to use class validator annotations in fields of a dto class that is responsible for returning data. EDIT: I used the single dto approach in an enterprise and messed up the code real bad due to this exact same problem. |
* chore(ci): Made pipeline fixes (#60) * Adding PR title and es lint, prettier, unit test and e2e test checks * Adding commands based on package.json * added husky * order checks * folder updates * folder updates * eslint check * replaced * Add pnpm * refactored installation for pnpm * push diff validate pr yml * deleted commitlintrc file * added makefile * update working directory of ci * update .gitignore * update ci * update ci --------- Co-authored-by: Bhavya Jain <[email protected]> Co-authored-by: Ben <[email protected]> * improve workflow * fixing pr title validation identation issue * update readme (#62) * test(api): Added cleanup test to user module (#61) * added afterALl in user test * update CI * Update README.md * Update README.md * added redis (#63) * refactor: Moved items of backend folder to root (#65) * refactored backend folder * fixed tests * fixed tests * fix: Migrate to ioredis package (#70) * migrate to ioredis * update lockfile * fixed tests * update CI workflow * Update README.md * Update README-backend.md * feat: write-review-page (#71) * Add joined at date for searched user * Update user search query to respect respect http standards * Enable cors * minor fixes * Correct relations * Check if users are connected when doing an user search * fix db structure * fix namings * feat: auth flow changes (#73) * Add joined at date for searched user * Update user search query to respect respect http standards * Enable cors * minor fixes * Correct relations * Check if users are connected when doing an user search * fix db structure * fix namings * Use passwordless signup * Return props needed for signin redirection * feat: Added feature to search by social profile (#77) * added feature to search by social profile * add linkedin profile fetching functionality * finishing touches * delete user controller test file * fixed inconsistencies in e2e tests * made search user endpoint public * feat: Emails are stored in lowercase (#82) * feat: Emails are stored in lowercase * organized code * fix: dashboard various fixes (#78) * Add joined at date for searched user * Update user search query to respect respect http standards * Correct relations * Check if users are connected when doing an user search * fix db structure * Fix: is connection should be true when userId is in the followers * Do not use mutler for uploading profile pics * Context: React native web sends base64 images that cannot be parsed by mutler * update review properties * add get user endpoint * use headline insted of jobtitle * remove console logs * remove console log * Move reviwes in dedicated controller. * Rename ratings to reviews * Use ratings only for professionalism/communication/reliability * Create reviews controller and service * Remove route with */self. * enable swagger plugin * review db structure changes: * change id from int to string * add state * add favorite review model * Add ability to like/unlike reviews and review state * add ability to modify and update own review * feat: add connections controller and move specific endpoints from user controller here * fix: review annonymous property * fix: add isEmailVerified to connection * fix rebase errors * Move search by profile url in connections. * Add authType property on connectionDto * remove jobTitle property. Use headline everywhere * Return transformed user when social account is already existen * add logging * fix reviews issues * rename dto folder * rename methods * make only one DB query for craeting a connection * change DTO folder to a random name (because gh is not case sensitive and doesn't see lowering the case of the file as an actual change) * rename dto folder * squash migrations * fix tests * feat: implement user settings endpoints (#84) * feat: user profile (#85) * fix: Docker builds were not running due to module errors (#86) * feat: implement push notifications (#87) * feat: implement push notifications * fix tests --------- Co-authored-by: Rajdip Bhattacharya <[email protected]> Co-authored-by: Bhavya Jain <[email protected]> Co-authored-by: Ben <[email protected]> Co-authored-by: Adelina Enache <[email protected]>
Fixes #80
Fixes #79
Fixes #74